Skip to content

majit: Defer Dynasm bridge constant conversion to diagnostic path - #1299

Merged
youknowone merged 1 commit into
youknowone:mainfrom
notJoon:refactor/lazy-dynasm-bridge-constants
Aug 17, 2026
Merged

majit: Defer Dynasm bridge constant conversion to diagnostic path#1299
youknowone merged 1 commit into
youknowone:mainfrom
notJoon:refactor/lazy-dynasm-bridge-constants

Conversation

@notJoon

@notJoon notJoon commented Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Move the raw constant map conversion into the PYRE_TRACE_OPS_DIAG branch, avoiding diagnostic-only allocation and iteration during normal bridge compilation.

compile_loop has similar const_pool collection logic, but I left it duplicated rather than extracting a helper that isn't needed yet.

pyre/majit/majit-backend-dynasm/src/runner.rs

Lines 2394 to 2397 in 21ce530

let constants: indexmap::IndexMap<u32, i64> = const_pool
.iter()
.map(|(&key, value)| (key, value.as_raw_i64()))
.collect();

Self-review

  • I fully resolved all reasonable code review comments from Codex and CodeRabbit.
    • Auto-review section 1 is clear. This check is mandatory.
    • Auto-review section 2 is clear. If this is not checked, please add a comment explaining why.
  • I did not use AI to write the code of this patch.
    • If this is not checked, commits must include Assisted-by

Summary by CodeRabbit

  • Refactor
    • Streamlined internal diagnostic handling without changing the resulting output or end-user behavior.

@coderabbitai

coderabbitai Bot commented Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 60e713c0-5a32-4701-85ba-86979673c057

📥 Commits

Reviewing files that changed from the base of the PR and between 21ce530 and 4ba378b.

📒 Files selected for processing (1)
  • majit/majit-backend-dynasm/src/runner.rs

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.

Walkthrough

The change reorders trace-diagnostic evaluation in compile_bridge and uses the imported IndexMap type for the raw constant map.

Changes

Compile bridge diagnostic ordering

Layer / File(s) Summary
Trace diagnostic setup
majit/majit-backend-dynasm/src/runner.rs		 compile_bridge checks the trace-diagnostic environment before constructing the raw constant map. The map now uses the imported IndexMap type.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: 🟡 Moderate · up to 4ba37

The Dynasm backend change is not merge-ready until the required checks run successfully or their omission is explicitly accepted; the current environment lacks the Rust toolchain needed to execute them.

Possibly related PRs

Suggested reviewers: lifthrasiir, youknowone

Poem

A rabbit checks the trace with care,
Then maps the constants in a tidy lair.
IndexMap hops into the stream,
The bridge now follows a cleaner dream.
Four lines changed; the burrow beams.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes deferring Dynasm bridge constant conversion to the diagnostic path.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 4ba378b).
Updated: 2026-08-17T12:02:23.044Z

Files in the reviewed diff 
majit/majit-backend-dynasm/src/runner.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

None.

youknowone
youknowone approved these changes Aug 17, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! and welcome to Pyre project!

@youknowone
youknowone merged commit 81e8a2d into youknowone:main Aug 17, 2026
17 of 19 checks passed
@notJoon
notJoon deleted the refactor/lazy-dynasm-bridge-constants branch August 17, 2026 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@notJoon @youknowone